-
Notifications
You must be signed in to change notification settings - Fork 27.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Incremental Static Regeneration - docs update explaining surprising behavior #66151
Conversation
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for submitting a PR!
One point in favor of this being a bug: The CLI reports that a route is SSG enabled in the build output, but doesn't actually cache post-build page renders if this export is missing.
Good callout! We need to I think we need to improve our CLI output here.
The requirement to export a generateStaticParams to get static cache behavior after build time was really surprising behavior for me, and I think others:
Hmm, maybe somewhat! The behavior is similar to Pages Router—if you only had a dynamic page, it would not opt you into ISR—you would have to use getStaticPath
s. Similarly, you have to use generateStaticParams
here.
A possible discussion to have is whether revalidate
should just enable it, without an empty generateStaticParams
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would love to get @delbaoliveira's opinion here before merging 🙏
@samcx I think my confusion here may stem from the fact that I didn't use pages router before. As more folks start to use next-js after app router is the recommended default, the level of surprise among users may rise ;) Thanks for taking a look at the PR 🙌 |
@karlkeefer Makes sense and noted! We are working to improve the App Router's caching story which will most likely involve improving the Route Segment Config, so we could end up improving this to cause less confusion. I'll defer to Delba for further review on this |
docs/02-app/02-api-reference/02-file-conventions/route-segment-config.mdx
Outdated
Show resolved
Hide resolved
docs/02-app/02-api-reference/04-functions/generate-static-params.mdx
Outdated
Show resolved
Hide resolved
Thank you for raising this @karlkeefer, I've pushed a commit to rephrase a few things. @samcx maybe not supplying |
Or setting |
@rijk Thanks for the feedback! Actually not 100% with |
@rijk It turns out you can use |
## Why? You can also utilize `export cont dynamic = 'force-static'` to ISR pages at runtime, instead of having to returning an empty array in `generateStaticParams`. x-ref: #66151, #62195 (comment)
The requirement to export a
generateStaticParams
to get static cache behavior after build time was really surprising behavior for me, and I think others:Potentially this is a bug, and not something that should be fixed with documentation?
I don't understand next.js caching enough to make that determination, so instead I'm proposing these changes to docs which might be encountered by folks who are surprised by this cache behavior.
One point in favor of this being a bug: The CLI reports that a route is
SSG
enabled in the build output, but doesn't actually cache post-build page renders if this export is missing.@awinogrodzki made a demo repo showing this behavior, as described here.